This repository has been archived by the owner on Dec 13, 2018. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 106
Saml integration #557
Open
the-overengineer
wants to merge
13
commits into
master
Choose a base branch
from
saml-integration
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Saml integration #557
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mheisig
reviewed
Jan 13, 2017
var application = req.app.get('stormpathApplication'); | ||
var builder = new stormpath.SamlIdpUrlBuilder(application); | ||
var config = req.app.get('stormpathConfig'); | ||
var cbUri = req.protocol + '://' + req.get('host') + config.web.saml.verifyUri; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using a similar approach to support SAML login. I'd recommend supporting a host
config option here instead of assuming the req.host
. Running this in a Docker container behind an nginx proxy winds up with the host
being reported as the Docker service name instead of the public URI.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements the SAML flow for traditional websites. The URL is built on-demand (to keep the timestamps current in the JWT) and the user is redirected to the provider, through the usual flow, and back to the configured URL, which then verifies the token and redirects the user appropriately.
All of this happens automatically if
web.saml.enabled
is set to true in the configuration.As for the SPA flow, it currently only tosses the provider data in the login form view model, which is the old behaviour. This does not include the idp url generation. @robertjd Should it? To preserve the same behaviour as the traditional websites, it would have to get a fresh token with the redirect URI.
The only idea that comes to mind without adding verification logic to the SPAs is to have the SPA also redirect the user to the
/verify
page, passing a query parameter so that the/saml-verify
endpoint knows to redirect the user back to the SPA.Alternatively, it could redirect back to the app and have it exchange it via
stormpath_token
grant, like the Client API social flow. In this case, all express would have to do is generate a fresh token in the initial redirect URL. Not sure what you intend to do with the Client API changes, as well.This part is still a WIP, obviously.
Also, we talked about route naming, so that's something that ought to be checked.
Fixes #492 (once the SPA part is done)